-
Notifications
You must be signed in to change notification settings - Fork 761
Convert sources reducer to ES modules #2736
Convert sources reducer to ES modules #2736
Conversation
Without Flow complained about missing annotation. After setting it to `SourcesState` it complained about unrecognized methods, which I suspect come from Immutable and are not defined and `SourcesState`. Since this PR isn't about type-checking, I left it at `any`.
Codecov Report
@@ Coverage Diff @@
## master #2736 +/- ##
==========================================
- Coverage 60.66% 60.58% -0.08%
==========================================
Files 55 55
Lines 2148 2144 -4
Branches 434 434
==========================================
- Hits 1303 1299 -4
Misses 845 845
Continue to review full report at Codecov.
|
@virzen I haven't looked yet, but you might want to use the |
src/reducers/sources.js
Outdated
@@ -44,7 +44,10 @@ const State = makeRecord( | |||
}: SourcesState) | |||
); | |||
|
|||
function update(state = State(), action: Action): Record<SourcesState> { | |||
export function update( | |||
state: any = State(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using state: Record<SourcesState>
should work.
src/reducers/sources.js
Outdated
@@ -44,7 +44,10 @@ const State = makeRecord( | |||
}: SourcesState) | |||
); | |||
|
|||
function update(state = State(), action: Action): Record<SourcesState> { | |||
export function update( | |||
state: any = State(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that you could replace any
with Record<SourcesState>
. Can you try this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, indeed works! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops looks like I was late 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hahaha, It's never too late :P
const makeRecord = require("../utils/makeRecord"); | ||
const { getPrettySourceURL } = require("../utils/source"); | ||
const { prefs } = require("../utils/prefs"); | ||
import * as I from "immutable"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably use import I from "immutable";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems not to be the case:
src/reducers/sources.js:11
11: import I from "immutable";
^ Default import from `immutable`. This module has no default export.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. Thanks for correction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's interesting. I wonder if this means we should only be importing the things we need from immutable
in the future. Thanks for pointing this out and looking it up. 🥂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although there would be no significant gain in size (webpack 1.x bundles whole file anyway) I agree it's a good practice to do so. Also, it would be great for the eventual migration to webpack 2 (which features tree-shaking). 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @virzen
🎉 No problem! |
* Convert reducers/sources to es modules * Convert tests for reducers/sources to es modules * Remove type for sources reducer's state * Set sources reducer's state argument's type to any Without Flow complained about missing annotation. After setting it to `SourcesState` it complained about unrecognized methods, which I suspect come from Immutable and are not defined and `SourcesState`. Since this PR isn't about type-checking, I left it at `any`. * Use Record notation for sources reducer's argument type
Associated Issue: #1884
Summary of Changes
any
to silence Flow errorNote about last point:
Without it Flow complained about missing annotation after addition of
export
statement. After setting it toSourcesState
it complainedabout unrecognized methods, which I suspect come from Immutable.
Since this PR isn't about type-checking, I left it at
any
, as suggestedin this comment.